From 49c83155ccb78284b17a9ffa47583ddace5dbd01 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Mon, 15 Jul 2019 19:59:57 +0200 Subject: [PATCH 1/3] Load search result counts asynchronously Querying all counts for the different search results in the same request led to timeouts, so we now only calculate the count for the *current* search results, and request the others in separate asynchronous calls. --- app/controllers/search_controller.rb | 9 + app/helpers/search_helper.rb | 35 ++- app/views/layouts/_search.html.haml | 1 + app/views/search/_category.html.haml | 84 +----- .../unreleased/load-search-counts-async.yml | 5 + config/routes.rb | 1 + lib/gitlab/project_search_results.rb | 15 ++ lib/gitlab/search_results.rb | 23 ++ lib/gitlab/snippet_search_results.rb | 11 + spec/controllers/search_controller_spec.rb | 244 ++++++++++-------- .../search/user_searches_for_users_spec.rb | 4 +- .../user_uses_header_search_field_spec.rb | 17 ++ spec/helpers/search_helper_spec.rb | 44 ++++ .../lib/gitlab/project_search_results_spec.rb | 22 ++ spec/lib/gitlab/search_results_spec.rb | 37 +++ .../lib/gitlab/snippet_search_results_spec.rb | 18 ++ 16 files changed, 373 insertions(+), 197 deletions(-) create mode 100644 changelogs/unreleased/load-search-counts-async.yml diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 13741548687..e30935be4b6 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -36,6 +36,15 @@ class SearchController < ApplicationController check_single_commit_result end + def count + params.require([:search, :scope]) + + scope = search_service.scope + count = search_service.search_results.formatted_count(scope) + + render json: { count: count } + end + # rubocop: disable CodeReuse/ActiveRecord def autocomplete term = params[:term] diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index f5c4686a3bf..add4e555c70 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -145,17 +145,28 @@ module SearchHelper Sanitize.clean(str) end - def search_filter_path(options = {}) - exist_opts = { - search: params[:search], - project_id: params[:project_id], - group_id: params[:group_id], - scope: params[:scope], - repository_ref: params[:repository_ref] - } + def search_filter_link(scope, label, data: {}, search: {}) + search_params = params + .merge(search) + .merge({ scope: scope }) + .permit(:search, :scope, :project_id, :group_id, :repository_ref, :snippets) - options = exist_opts.merge(options) - search_path(options) + if @scope == scope + li_class = 'active' + count = @search_results.formatted_count(scope) + else + count = 0 + badge_class = 'js-search-count' + badge_data = { scope: scope, url: search_count_path(search_params) } + end + + content_tag :li, class: li_class, data: data do + link_to search_path(search_params) do + concat label + concat ' ' + concat content_tag(:span, count, class: 'badge badge-pill', data: { scope: scope }) + end + end end def search_filter_input_options(type) @@ -212,10 +223,6 @@ module SearchHelper sanitize(html, tags: %w(a p ol ul li pre code)) end - def limited_count(count, limit = 1000) - count > limit ? "#{limit}+" : count - end - def search_tabs?(tab) return false if Feature.disabled?(:users_search, default_enabled: true) diff --git a/app/views/layouts/_search.html.haml b/app/views/layouts/_search.html.haml index bc900992cb0..e6a235e39da 100644 --- a/app/views/layouts/_search.html.haml +++ b/app/views/layouts/_search.html.haml @@ -47,5 +47,6 @@ = hidden_field_tag :snippets, true = hidden_field_tag :repository_ref, @ref = hidden_field_tag :nav_source, 'navbar' + -# workaround for non-JS feature specs, for JS you need to use find('#search').send_keys(:enter) = button_tag 'Go' if ENV['RAILS_ENV'] == 'test' .search-autocomplete-opts.hide{ :'data-autocomplete-path' => search_autocomplete_path, :'data-autocomplete-project-id' => @project.try(:id), :'data-autocomplete-project-ref' => @ref } diff --git a/app/views/search/_category.html.haml b/app/views/search/_category.html.haml index 18613ff4c16..eae2a491ceb 100644 --- a/app/views/search/_category.html.haml +++ b/app/views/search/_category.html.haml @@ -1,10 +1,6 @@ - users = capture_haml do - if search_tabs?(:members) - %li{ class: active_when(@scope == 'users') } - = link_to search_filter_path(scope: 'users') do - Users - %span.badge.badge-pill - = limited_count(@search_results.limited_users_count) + = search_filter_link 'users', _("Users") .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller .fade-left= icon('angle-left') @@ -12,80 +8,28 @@ %ul.nav-links.search-filter.scrolling-tabs.nav.nav-tabs - if @project - if project_search_tabs?(:blobs) - %li{ class: active_when(@scope == 'blobs'), data: { qa_selector: 'code_tab' } } - = link_to search_filter_path(scope: 'blobs') do - = _("Code") - %span.badge.badge-pill - = @search_results.blobs_count + = search_filter_link 'blobs', _("Code"), data: { qa_selector: 'code_tab' } - if project_search_tabs?(:issues) - %li{ class: active_when(@scope == 'issues') } - = link_to search_filter_path(scope: 'issues') do - = _("Issues") - %span.badge.badge-pill - = limited_count(@search_results.limited_issues_count) + = search_filter_link 'issues', _("Issues") - if project_search_tabs?(:merge_requests) - %li{ class: active_when(@scope == 'merge_requests') } - = link_to search_filter_path(scope: 'merge_requests') do - = _("Merge requests") - %span.badge.badge-pill - = limited_count(@search_results.limited_merge_requests_count) + = search_filter_link 'merge_requests', _("Merge requests") - if project_search_tabs?(:milestones) - %li{ class: active_when(@scope == 'milestones') } - = link_to search_filter_path(scope: 'milestones') do - = _("Milestones") - %span.badge.badge-pill - = limited_count(@search_results.limited_milestones_count) + = search_filter_link 'milestones', _("Milestones") - if project_search_tabs?(:notes) - %li{ class: active_when(@scope == 'notes') } - = link_to search_filter_path(scope: 'notes') do - = _("Comments") - %span.badge.badge-pill - = limited_count(@search_results.limited_notes_count) + = search_filter_link 'notes', _("Comments") - if project_search_tabs?(:wiki) - %li{ class: active_when(@scope == 'wiki_blobs') } - = link_to search_filter_path(scope: 'wiki_blobs') do - = _("Wiki") - %span.badge.badge-pill - = @search_results.wiki_blobs_count + = search_filter_link 'wiki_blobs', _("Wiki") - if project_search_tabs?(:commits) - %li{ class: active_when(@scope == 'commits') } - = link_to search_filter_path(scope: 'commits') do - = _("Commits") - %span.badge.badge-pill - = @search_results.commits_count + = search_filter_link 'commits', _("Commits") = users - elsif @show_snippets - %li{ class: active_when(@scope == 'snippet_blobs') } - = link_to search_filter_path(scope: 'snippet_blobs', snippets: true, group_id: nil, project_id: nil) do - = _("Snippet Contents") - %span.badge.badge-pill - = @search_results.snippet_blobs_count - %li{ class: active_when(@scope == 'snippet_titles') } - = link_to search_filter_path(scope: 'snippet_titles', snippets: true, group_id: nil, project_id: nil) do - = _("Titles and Filenames") - %span.badge.badge-pill - = @search_results.snippet_titles_count + = search_filter_link 'snippet_blobs', _("Snippet Contents"), search: { snippets: true, group_id: nil, project_id: nil } + = search_filter_link 'snippet_titles', _("Titles and Filenames"), search: { snippets: true, group_id: nil, project_id: nil } - else - %li{ class: active_when(@scope == 'projects') } - = link_to search_filter_path(scope: 'projects') do - = _("Projects") - %span.badge.badge-pill - = limited_count(@search_results.limited_projects_count) - %li{ class: active_when(@scope == 'issues') } - = link_to search_filter_path(scope: 'issues') do - = _("Issues") - %span.badge.badge-pill - = limited_count(@search_results.limited_issues_count) - %li{ class: active_when(@scope == 'merge_requests') } - = link_to search_filter_path(scope: 'merge_requests') do - = _("Merge requests") - %span.badge.badge-pill - = limited_count(@search_results.limited_merge_requests_count) - %li{ class: active_when(@scope == 'milestones') } - = link_to search_filter_path(scope: 'milestones') do - = _("Milestones") - %span.badge.badge-pill - = limited_count(@search_results.limited_milestones_count) + = search_filter_link 'projects', _("Projects") + = search_filter_link 'issues', _("Issues") + = search_filter_link 'merge_requests', _("Merge requests") + = search_filter_link 'milestones', _("Milestones") = render_if_exists 'search/category_elasticsearch' = users diff --git a/changelogs/unreleased/load-search-counts-async.yml b/changelogs/unreleased/load-search-counts-async.yml new file mode 100644 index 00000000000..1f466450e76 --- /dev/null +++ b/changelogs/unreleased/load-search-counts-async.yml @@ -0,0 +1,5 @@ +--- +title: Load search result counts asynchronously +merge_request: 31663 +author: +type: changed diff --git a/config/routes.rb b/config/routes.rb index 459f2b22bf0..fdef31429f3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -58,6 +58,7 @@ Rails.application.routes.draw do # Search get 'search' => 'search#show' get 'search/autocomplete' => 'search#autocomplete', as: :search_autocomplete + get 'search/count' => 'search#count', as: :search_count # JSON Web Token get 'jwt/auth' => 'jwt#auth' diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index 5e77d31760d..2669adb8455 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -29,6 +29,21 @@ module Gitlab end end + def formatted_count(scope) + case scope + when 'blobs' + blobs_count.to_s + when 'notes' + formatted_limited_count(limited_notes_count) + when 'wiki_blobs' + wiki_blobs_count.to_s + when 'commits' + commits_count.to_s + else + super + end + end + def users super.where(id: @project.team.members) # rubocop:disable CodeReuse/ActiveRecord end diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 7c1e6b1baff..ce4c1611687 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -43,6 +43,29 @@ module Gitlab without_count ? collection.without_count : collection end + def formatted_count(scope) + case scope + when 'projects' + formatted_limited_count(limited_projects_count) + when 'issues' + formatted_limited_count(limited_issues_count) + when 'merge_requests' + formatted_limited_count(limited_merge_requests_count) + when 'milestones' + formatted_limited_count(limited_milestones_count) + when 'users' + formatted_limited_count(limited_users_count) + end + end + + def formatted_limited_count(count) + if count >= COUNT_LIMIT + "#{COUNT_LIMIT - 1}+" + else + count.to_s + end + end + def limited_projects_count @limited_projects_count ||= limited_count(projects) end diff --git a/lib/gitlab/snippet_search_results.rb b/lib/gitlab/snippet_search_results.rb index e360b552f89..ac3b219e0c7 100644 --- a/lib/gitlab/snippet_search_results.rb +++ b/lib/gitlab/snippet_search_results.rb @@ -22,6 +22,17 @@ module Gitlab end end + def formatted_count(scope) + case scope + when 'snippet_titles' + snippet_titles_count.to_s + when 'snippet_blobs' + snippet_blobs_count.to_s + else + super + end + end + def snippet_titles_count @snippet_titles_count ||= snippet_titles.count end diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 5a5c0a1f6ac..3e0d53a6573 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -11,118 +11,27 @@ describe SearchController do sign_in(user) end - context 'uses the right partials depending on scope' do - using RSpec::Parameterized::TableSyntax - render_views - - set(:project) { create(:project, :public, :repository, :wiki_repo) } - - before do - expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original - end - - subject { get(:show, params: { project_id: project.id, scope: scope, search: 'merge' }) } - - where(:partial, :scope) do - '_blob' | :blobs - '_wiki_blob' | :wiki_blobs - '_commit' | :commits - end - - with_them do - it do - project_wiki = create(:project_wiki, project: project, user: user) - create(:wiki_page, wiki: project_wiki, attrs: { title: 'merge', content: 'merge' }) - - expect(subject).to render_template("search/results/#{partial}") - end - end - end - - context 'global search' do - render_views - - it 'omits pipeline status from load' do - project = create(:project, :public) - expect(Gitlab::Cache::Ci::ProjectPipelineStatus).not_to receive(:load_in_batch_for_projects) - - get :show, params: { scope: 'projects', search: project.name } - - expect(assigns[:search_objects].first).to eq project - end - end - - it 'finds issue comments' do - project = create(:project, :public) - note = create(:note_on_issue, project: project) - - get :show, params: { project_id: project.id, scope: 'notes', search: note.note } - - expect(assigns[:search_objects].first).to eq note - end - - context 'when the user cannot read cross project' do + shared_examples_for 'when the user cannot read cross project' do |action, params| before do allow(Ability).to receive(:allowed?).and_call_original allow(Ability).to receive(:allowed?) .with(user, :read_cross_project, :global) { false } end - it 'still allows accessing the search page' do - get :show - - expect(response).to have_gitlab_http_status(200) - end - - it 'still blocks searches without a project_id' do - get :show, params: { search: 'hello' } + it 'blocks access without a project_id' do + get action, params: params expect(response).to have_gitlab_http_status(403) end - it 'allows searches with a project_id' do - get :show, params: { search: 'hello', project_id: create(:project, :public).id } + it 'allows access with a project_id' do + get action, params: params.merge(project_id: create(:project, :public).id) expect(response).to have_gitlab_http_status(200) end end - context 'on restricted projects' do - context 'when signed out' do - before do - sign_out(user) - end - - it "doesn't expose comments on issues" do - project = create(:project, :public, :issues_private) - note = create(:note_on_issue, project: project) - - get :show, params: { project_id: project.id, scope: 'notes', search: note.note } - - expect(assigns[:search_objects].count).to eq(0) - end - end - - it "doesn't expose comments on merge_requests" do - project = create(:project, :public, :merge_requests_private) - note = create(:note_on_merge_request, project: project) - - get :show, params: { project_id: project.id, scope: 'notes', search: note.note } - - expect(assigns[:search_objects].count).to eq(0) - end - - it "doesn't expose comments on snippets" do - project = create(:project, :public, :snippets_private) - note = create(:note_on_project_snippet, project: project) - - get :show, params: { project_id: project.id, scope: 'notes', search: note.note } - - expect(assigns[:search_objects].count).to eq(0) - end - end - - context 'with external authorization service enabled' do + shared_examples_for 'with external authorization service enabled' do |action, params| let(:project) { create(:project, namespace: user.namespace) } let(:note) { create(:note_on_issue, project: project) } @@ -130,32 +39,145 @@ describe SearchController do enable_external_authorization_service_check end - describe 'GET #show' do - it 'renders a 403 when no project is given' do - get :show, params: { scope: 'notes', search: note.note } + it 'renders a 403 when no project is given' do + get action, params: params - expect(response).to have_gitlab_http_status(403) - end + expect(response).to have_gitlab_http_status(403) + end - it 'renders a 200 when a project was set' do - get :show, params: { project_id: project.id, scope: 'notes', search: note.note } + it 'renders a 200 when a project was set' do + get action, params: params.merge(project_id: project.id) + + expect(response).to have_gitlab_http_status(200) + end + end + + describe 'GET #show' do + it_behaves_like 'when the user cannot read cross project', :show, { search: 'hello' } do + it 'still allows accessing the search page' do + get :show expect(response).to have_gitlab_http_status(200) end end - describe 'GET #autocomplete' do - it 'renders a 403 when no project is given' do - get :autocomplete, params: { term: 'hello' } + it_behaves_like 'with external authorization service enabled', :show, { search: 'hello' } - expect(response).to have_gitlab_http_status(403) + context 'uses the right partials depending on scope' do + using RSpec::Parameterized::TableSyntax + render_views + + set(:project) { create(:project, :public, :repository, :wiki_repo) } + + before do + expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original end - it 'renders a 200 when a project was set' do - get :autocomplete, params: { project_id: project.id, term: 'hello' } + subject { get(:show, params: { project_id: project.id, scope: scope, search: 'merge' }) } - expect(response).to have_gitlab_http_status(200) + where(:partial, :scope) do + '_blob' | :blobs + '_wiki_blob' | :wiki_blobs + '_commit' | :commits + end + + with_them do + it do + project_wiki = create(:project_wiki, project: project, user: user) + create(:wiki_page, wiki: project_wiki, attrs: { title: 'merge', content: 'merge' }) + + expect(subject).to render_template("search/results/#{partial}") + end + end + end + + context 'global search' do + render_views + + it 'omits pipeline status from load' do + project = create(:project, :public) + expect(Gitlab::Cache::Ci::ProjectPipelineStatus).not_to receive(:load_in_batch_for_projects) + + get :show, params: { scope: 'projects', search: project.name } + + expect(assigns[:search_objects].first).to eq project + end + end + + it 'finds issue comments' do + project = create(:project, :public) + note = create(:note_on_issue, project: project) + + get :show, params: { project_id: project.id, scope: 'notes', search: note.note } + + expect(assigns[:search_objects].first).to eq note + end + + context 'on restricted projects' do + context 'when signed out' do + before do + sign_out(user) + end + + it "doesn't expose comments on issues" do + project = create(:project, :public, :issues_private) + note = create(:note_on_issue, project: project) + + get :show, params: { project_id: project.id, scope: 'notes', search: note.note } + + expect(assigns[:search_objects].count).to eq(0) + end + end + + it "doesn't expose comments on merge_requests" do + project = create(:project, :public, :merge_requests_private) + note = create(:note_on_merge_request, project: project) + + get :show, params: { project_id: project.id, scope: 'notes', search: note.note } + + expect(assigns[:search_objects].count).to eq(0) + end + + it "doesn't expose comments on snippets" do + project = create(:project, :public, :snippets_private) + note = create(:note_on_project_snippet, project: project) + + get :show, params: { project_id: project.id, scope: 'notes', search: note.note } + + expect(assigns[:search_objects].count).to eq(0) end end end + + describe 'GET #count' do + it_behaves_like 'when the user cannot read cross project', :count, { search: 'hello', scope: 'projects' } + it_behaves_like 'with external authorization service enabled', :count, { search: 'hello', scope: 'projects' } + + it 'returns the result count for the given term and scope' do + create(:project, :public, name: 'hello world') + create(:project, :public, name: 'foo bar') + + get :count, params: { search: 'hello', scope: 'projects' } + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to eq({ 'count' => '1' }) + end + + it 'raises an error if search term is missing' do + expect do + get :count, params: { scope: 'projects' } + end.to raise_error(ActionController::ParameterMissing) + end + + it 'raises an error if search scope is missing' do + expect do + get :count, params: { search: 'hello' } + end.to raise_error(ActionController::ParameterMissing) + end + end + + describe 'GET #autocomplete' do + it_behaves_like 'when the user cannot read cross project', :autocomplete, { term: 'hello' } + it_behaves_like 'with external authorization service enabled', :autocomplete, { term: 'hello' } + end end diff --git a/spec/features/search/user_searches_for_users_spec.rb b/spec/features/search/user_searches_for_users_spec.rb index 2517a843c62..e10c1afc0b8 100644 --- a/spec/features/search/user_searches_for_users_spec.rb +++ b/spec/features/search/user_searches_for_users_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' describe 'User searches for users' do context 'when on the dashboard' do - it 'finds the user' do + it 'finds the user', :js do create(:user, username: 'gob_bluth', name: 'Gob Bluth') sign_in(create(:user)) @@ -12,7 +12,7 @@ describe 'User searches for users' do visit dashboard_projects_path fill_in 'search', with: 'gob' - click_button 'Go' + find('#search').send_keys(:enter) expect(page).to have_content('Users 1') 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 c781048d06d..5006631cc14 100644 --- a/spec/features/search/user_uses_header_search_field_spec.rb +++ b/spec/features/search/user_uses_header_search_field_spec.rb @@ -96,6 +96,23 @@ describe 'User uses header search field', :js do let(:url) { root_path } let(:scope_name) { 'All GitLab' } end + + context 'when searching through the search field' do + before do + create(:issue, project: project, title: 'project issue') + + fill_in('search', with: 'project') + find('#search').send_keys(:enter) + end + + it 'displays result counts for all categories' do + expect(page).to have_content('Projects 1') + expect(page).to have_content('Issues 1') + expect(page).to have_content('Merge requests 0') + expect(page).to have_content('Milestones 0') + expect(page).to have_content('Users 0') + end + end end context 'when user is in a project scope' do diff --git a/spec/helpers/search_helper_spec.rb b/spec/helpers/search_helper_spec.rb index c69493b579f..2ab72679ee7 100644 --- a/spec/helpers/search_helper_spec.rb +++ b/spec/helpers/search_helper_spec.rb @@ -177,4 +177,48 @@ describe SearchHelper do end end end + + describe 'search_filter_link' do + it 'renders a search filter link for the current scope' do + @scope = 'projects' + @search_results = double + + expect(@search_results).to receive(:formatted_count).with('projects').and_return('23') + + link = search_filter_link('projects', 'Projects') + + expect(link).to have_css('li.active') + expect(link).to have_link('Projects', href: search_path(scope: 'projects')) + expect(link).to have_css('span.badge.badge-pill:not(.js-search-count):not(.hidden):not([data-url])', text: '23') + end + + it 'renders a search filter link for another scope' do + link = search_filter_link('projects', 'Projects') + count_path = search_count_path(scope: 'projects') + + expect(link).to have_css('li:not([class="active"])') + expect(link).to have_link('Projects', href: search_path(scope: 'projects')) + expect(link).to have_css("span.badge.badge-pill.js-search-count.hidden[data-url='#{count_path}']", text: '') + end + + it 'merges in the current search params and given params' do + expect(self).to receive(:params).and_return( + ActionController::Parameters.new( + search: 'hello', + scope: 'ignored', + other_param: 'ignored' + ) + ) + + link = search_filter_link('projects', 'Projects', search: { project_id: 23 }) + + expect(link).to have_link('Projects', href: search_path(scope: 'projects', search: 'hello', project_id: 23)) + end + + it 'assigns given data attributes on the list container' do + link = search_filter_link('projects', 'Projects', data: { foo: 'bar' }) + + expect(link).to have_css('li[data-foo="bar"]') + end + end end diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb index 4a41d5cf51e..c7462500c82 100644 --- a/spec/lib/gitlab/project_search_results_spec.rb +++ b/spec/lib/gitlab/project_search_results_spec.rb @@ -22,6 +22,28 @@ describe Gitlab::ProjectSearchResults do it { expect(results.query).to eq('hello world') } end + describe '#formatted_count' do + using RSpec::Parameterized::TableSyntax + + let(:results) { described_class.new(user, project, query) } + + where(:scope, :count_method, :expected) do + 'blobs' | :blobs_count | '1234' + 'notes' | :limited_notes_count | '1000+' + 'wiki_blobs' | :wiki_blobs_count | '1234' + 'commits' | :commits_count | '1234' + 'projects' | :limited_projects_count | '1000+' + 'unknown' | nil | nil + end + + with_them do + it 'returns the expected formatted count' do + expect(results).to receive(count_method).and_return(1234) if count_method + expect(results.formatted_count(scope)).to eq(expected) + end + end + end + shared_examples 'general blob search' do |entity_type, blob_kind| let(:query) { 'files' } subject(:results) { described_class.new(user, project, query).objects(blob_type) } diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index 3d27156b356..c287da19343 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -29,6 +29,43 @@ describe Gitlab::SearchResults do end end + describe '#formatted_count' do + using RSpec::Parameterized::TableSyntax + + where(:scope, :count_method, :expected) do + 'projects' | :limited_projects_count | '1000+' + 'issues' | :limited_issues_count | '1000+' + 'merge_requests' | :limited_merge_requests_count | '1000+' + 'milestones' | :limited_milestones_count | '1000+' + 'users' | :limited_users_count | '1000+' + 'unknown' | nil | nil + end + + with_them do + it 'returns the expected formatted count' do + expect(results).to receive(count_method).and_return(1234) if count_method + expect(results.formatted_count(scope)).to eq(expected) + end + end + end + + describe '#formatted_limited_count' do + using RSpec::Parameterized::TableSyntax + + where(:count, :expected) do + 23 | '23' + 1000 | '1000' + 1001 | '1000+' + 1234 | '1000+' + end + + with_them do + it 'returns the expected formatted limited count' do + expect(results.formatted_limited_count(count)).to eq(expected) + end + end + end + context "when count_limit is lower than total amount" do before do allow(results).to receive(:count_limit).and_return(1) diff --git a/spec/lib/gitlab/snippet_search_results_spec.rb b/spec/lib/gitlab/snippet_search_results_spec.rb index b661a894c0c..35df38f052b 100644 --- a/spec/lib/gitlab/snippet_search_results_spec.rb +++ b/spec/lib/gitlab/snippet_search_results_spec.rb @@ -16,4 +16,22 @@ describe Gitlab::SnippetSearchResults do expect(results.snippet_blobs_count).to eq(1) end end + + describe '#formatted_count' do + using RSpec::Parameterized::TableSyntax + + where(:scope, :count_method, :expected) do + 'snippet_titles' | :snippet_titles_count | '1234' + 'snippet_blobs' | :snippet_blobs_count | '1234' + 'projects' | :limited_projects_count | '1000+' + 'unknown' | nil | nil + end + + with_them do + it 'returns the expected formatted count' do + expect(results).to receive(count_method).and_return(1234) if count_method + expect(results.formatted_count(scope)).to eq(expected) + end + end + end end From 68aab284a11a3c5b261263be9f6ed914d1f46423 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Mon, 5 Aug 2019 02:20:52 -0500 Subject: [PATCH 2/3] FE fetch counts async on search page load Creates `refresh_counts` module to dynamically fetch and load data based on attributes of HAML elements. --- .../pages/search/show/refresh_counts.js | 24 +++++++++++++ .../javascripts/pages/search/show/search.js | 2 ++ app/helpers/search_helper.rb | 7 ++-- .../__snapshots__/refresh_counts_spec.js.snap | 7 ++++ .../pages/search/show/refresh_counts_spec.js | 35 +++++++++++++++++++ 5 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/pages/search/show/refresh_counts.js create mode 100644 spec/frontend/pages/search/show/__snapshots__/refresh_counts_spec.js.snap create mode 100644 spec/frontend/pages/search/show/refresh_counts_spec.js diff --git a/app/assets/javascripts/pages/search/show/refresh_counts.js b/app/assets/javascripts/pages/search/show/refresh_counts.js new file mode 100644 index 00000000000..fa75ee6075d --- /dev/null +++ b/app/assets/javascripts/pages/search/show/refresh_counts.js @@ -0,0 +1,24 @@ +import axios from '~/lib/utils/axios_utils'; + +function showCount(el, count) { + el.textContent = count; + el.classList.remove('hidden'); +} + +function refreshCount(el) { + const { url } = el.dataset; + + return axios + .get(url) + .then(({ data }) => showCount(el, data.count)) + .catch(e => { + // eslint-disable-next-line no-console + console.error(`Failed to fetch search count from '${url}'.`, e); + }); +} + +export default function refreshCounts() { + const elements = Array.from(document.querySelectorAll('.js-search-count')); + + return Promise.all(elements.map(refreshCount)); +} diff --git a/app/assets/javascripts/pages/search/show/search.js b/app/assets/javascripts/pages/search/show/search.js index 81b6225cb18..86ec78e1df8 100644 --- a/app/assets/javascripts/pages/search/show/search.js +++ b/app/assets/javascripts/pages/search/show/search.js @@ -3,6 +3,7 @@ import Flash from '~/flash'; import Api from '~/api'; import { __ } from '~/locale'; import Project from '~/pages/projects/project'; +import refreshCounts from './refresh_counts'; export default class Search { constructor() { @@ -14,6 +15,7 @@ export default class Search { this.groupId = $groupDropdown.data('groupId'); this.eventListeners(); + refreshCounts(); $groupDropdown.glDropdown({ selectable: true, diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index add4e555c70..91c83380b62 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -155,16 +155,15 @@ module SearchHelper li_class = 'active' count = @search_results.formatted_count(scope) else - count = 0 - badge_class = 'js-search-count' - badge_data = { scope: scope, url: search_count_path(search_params) } + badge_class = 'js-search-count hidden' + badge_data = { url: search_count_path(search_params) } end content_tag :li, class: li_class, data: data do link_to search_path(search_params) do concat label concat ' ' - concat content_tag(:span, count, class: 'badge badge-pill', data: { scope: scope }) + concat content_tag(:span, count, class: ['badge badge-pill', badge_class], data: badge_data) end end end diff --git a/spec/frontend/pages/search/show/__snapshots__/refresh_counts_spec.js.snap b/spec/frontend/pages/search/show/__snapshots__/refresh_counts_spec.js.snap new file mode 100644 index 00000000000..ce456d6c899 --- /dev/null +++ b/spec/frontend/pages/search/show/__snapshots__/refresh_counts_spec.js.snap @@ -0,0 +1,7 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`pages/search/show/refresh_counts fetches and displays search counts 1`] = ` +"
22
+
4
+
5
" +`; diff --git a/spec/frontend/pages/search/show/refresh_counts_spec.js b/spec/frontend/pages/search/show/refresh_counts_spec.js new file mode 100644 index 00000000000..ead268b3971 --- /dev/null +++ b/spec/frontend/pages/search/show/refresh_counts_spec.js @@ -0,0 +1,35 @@ +import MockAdapter from 'axios-mock-adapter'; +import { TEST_HOST } from 'helpers/test_constants'; +import axios from '~/lib/utils/axios_utils'; +import refreshCounts from '~/pages/search/show/refresh_counts'; + +const URL = `${TEST_HOST}/search/count?search=lorem+ipsum&project_id=3`; +const urlWithScope = scope => `${URL}&scope=${scope}`; +const counts = [{ scope: 'issues', count: 4 }, { scope: 'merge_requests', count: 5 }]; +const fixture = `
22
+ +`; + +describe('pages/search/show/refresh_counts', () => { + let mock; + + beforeEach(() => { + mock = new MockAdapter(axios); + setFixtures(fixture); + }); + + afterEach(() => { + mock.restore(); + }); + + it('fetches and displays search counts', () => { + counts.forEach(({ scope, count }) => { + mock.onGet(urlWithScope(scope)).reply(200, { count }); + }); + + // assert before act behavior + return refreshCounts().then(() => { + expect(document.body.innerHTML).toMatchSnapshot(); + }); + }); +}); From 2f8709fb53137c2f53409f2400cd85083b06d6f6 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 7 Aug 2019 22:22:02 +0200 Subject: [PATCH 3/3] Fix deprecation warning for dangerous order usage --- app/models/user.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index ac83c8e3256..374e00987c5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -438,18 +438,20 @@ class User < ApplicationRecord order = <<~SQL CASE - WHEN users.name = %{query} THEN 0 - WHEN users.username = %{query} THEN 1 - WHEN users.email = %{query} THEN 2 + WHEN users.name = :query THEN 0 + WHEN users.username = :query THEN 1 + WHEN users.email = :query THEN 2 ELSE 3 END SQL + sanitized_order_sql = Arel.sql(sanitize_sql_array([order, query: query])) + where( fuzzy_arel_match(:name, query, lower_exact_match: true) .or(fuzzy_arel_match(:username, query, lower_exact_match: true)) .or(arel_table[:email].eq(query)) - ).reorder(order % { query: ApplicationRecord.connection.quote(query) }, :name) + ).reorder(sanitized_order_sql, :name) end # Limits the result set to users _not_ in the given query/list of IDs.