Merge branch 'improve-search-page' into 'master'

Improve search page

Fixes #1270

* added pagination support
* filter results by type
* when switch between context - search params stays
* redesign comment search results
* search Issues and MR by description too

## Screenshots:

![Screenshot 2014-08-27 09.56.18](https://dev.gitlab.org/uploads/gitlab/gitlabhq/2053220cd4/Screenshot_2014-08-27_09.56.18.png)

![Screenshot 2014-08-27 09.56.00](https://dev.gitlab.org/uploads/gitlab/gitlabhq/726c764da4/Screenshot_2014-08-27_09.56.00.png)

See merge request !1043

app/assets/stylesheets/generic/common.scss

@@ -128,7 +128,7 @@ p.time {
 }
 
 .highlight_word {
-  border-bottom: 2px solid #F90;
+  background: #fafe3d;
 }
 
 .thin_area{

app/controllers/projects/issues_controller.rb

@@ -20,7 +20,7 @@ class Projects::IssuesController < Projects::ApplicationController
     terms = params['issue_search']
 
     @issues = issues_filtered
-    @issues = @issues.where("title LIKE ? OR description LIKE ?", "%#{terms}%", "%#{terms}%") if terms.present?
+    @issues = @issues.full_search(terms) if terms.present?
     @issues = @issues.page(params[:page]).per(20)
 
     assignee_id, milestone_id = params[:assignee_id], params[:milestone_id]

app/controllers/search_controller.rb

@@ -4,14 +4,25 @@ class SearchController < ApplicationController
   def show
     @project = Project.find_by(id: params[:project_id]) if params[:project_id].present?
     @group = Group.find_by(id: params[:group_id]) if params[:group_id].present?
+    @scope = params[:scope]
 
-    if @project
+    @search_results = if @project
                         return access_denied! unless can?(current_user, :download_code, @project)
 
-      @search_results = Search::ProjectService.new(@project, current_user, params).execute
+                        unless %w(blobs notes issues merge_requests).include?(@scope)
+                          @scope = 'blobs'
+                        end
+
+                        Search::ProjectService.new(@project, current_user, params).execute
                       else
-      @search_results = Search::GlobalService.new(current_user, params).execute
+                        unless %w(projects issues merge_requests).include?(@scope)
+                          @scope = 'projects'
                         end
+
+                        Search::GlobalService.new(current_user, params).execute
+                      end
+
+    @objects = @search_results.objects(@scope, params[:page])
   end
 
   def autocomplete

app/helpers/search_helper.rb

@@ -91,4 +91,21 @@ module SearchHelper
   def search_result_sanitize(str)
     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]
+    }
+
+    options = exist_opts.merge(options)
+    search_path(options)
+  end
+
+  # Sanitize html generated after parsing markdown from issue description or comment
+  def search_md_sanitize(html)
+    sanitize(html, tags: %w(a p ul li pre code))
+  end
 end

app/models/concerns/issuable.rb

@@ -49,6 +49,10 @@ module Issuable
       where("LOWER(title) like :query", query: "%#{query.downcase}%")
     end
 
+    def full_search(query)
+      where("LOWER(title) like :query OR LOWER(description) like :query", query: "%#{query.downcase}%")
+    end
+
     def sort(method)
       case method.to_s
       when 'newest' then reorder("#{table_name}.created_at DESC")

app/models/note.rb

@@ -146,6 +146,10 @@ class Note < ActiveRecord::Base
     def cross_reference_exists?(noteable, mentioner)
       where(noteable_id: noteable.id, system: true, note: "_mentioned in #{mentioner.gfm_reference}_").any?
     end
+
+    def search(query)
+      where("note like :query", query: "%#{query}%")
+    end
   end
 
   def commit_author

app/models/project.rb

@@ -177,11 +177,11 @@ class Project < ActiveRecord::Base
       joins(:issues, :notes, :merge_requests).order("issues.created_at, notes.created_at, merge_requests.created_at DESC")
     end
 
-    def search query
+    def search(query)
       joins(:namespace).where("projects.archived = ?", false).where("projects.name LIKE :query OR projects.path LIKE :query OR namespaces.name LIKE :query OR projects.description LIKE :query", query: "%#{query}%")
     end
 
-    def search_by_title query
+    def search_by_title(query)
       where("projects.archived = ?", false).where("LOWER(projects.name) LIKE :query", query: "%#{query.downcase}%")
     end
 

app/services/search/global_service.rb

@@ -7,30 +7,12 @@ module Search
     end
 
     def execute
-      query = params[:search]
-      query = Shellwords.shellescape(query) if query.present?
-      return result unless query.present?
-
       group = Group.find_by(id: params[:group_id]) if params[:group_id].present?
       projects = ProjectsFinder.new.execute(current_user)
       projects = projects.where(namespace_id: group.id) if group
       project_ids = projects.pluck(:id)
 
-      result[:projects] = projects.search(query).limit(20)
-      result[:merge_requests] = MergeRequest.in_projects(project_ids).search(query).order('updated_at DESC').limit(20)
-      result[:issues] = Issue.where(project_id: project_ids).search(query).order('updated_at DESC').limit(20)
-      result[:total_results] = %w(projects issues merge_requests).sum { |items| result[items.to_sym].size }
-      result
-    end
-
-    def result
-      @result ||= {
-        projects: [],
-        merge_requests: [],
-        issues: [],
-        notes: [],
-        total_results: 0,
-      }
+      Gitlab::SearchResults.new(project_ids, params[:search])
     end
   end
 end

app/services/search/project_service.rb

@@ -7,39 +7,9 @@ module Search
     end
 
     def execute
-      query = params[:search]
-      query = Shellwords.shellescape(query) if query.present?
-      return result unless query.present?
-
-      if params[:search_code].present?
-        if !@project.empty_repo?
-          blobs = project.repository.search_files(query,
+      Gitlab::ProjectSearchResults.new(project.id,
+                                       params[:search],
                                        params[:repository_ref])
-        else
-          blobs = Array.new
-        end
-
-        blobs = Kaminari.paginate_array(blobs).page(params[:page]).per(20)
-        result[:blobs] = blobs
-        result[:total_results] = blobs.total_count
-      else
-        result[:merge_requests] = project.merge_requests.search(query).order('updated_at DESC').limit(20)
-        result[:issues] = project.issues.where("title like :query OR description like :query ", query: "%#{query}%").order('updated_at DESC').limit(20)
-        result[:notes] = Note.where(noteable_type: 'issue').where(project_id: project.id).where("note like :query", query: "%#{query}%").order('updated_at DESC').limit(20)
-        result[:total_results] = %w(issues merge_requests notes).sum { |items| result[items.to_sym].size }
-      end
-
-      result
-    end
-
-    def result
-      @result ||= {
-        merge_requests: [],
-        issues: [],
-        blobs: [],
-        notes: [],
-        total_results: 0,
-      }
     end
   end
 end

app/views/search/_filter.html.haml

@@ -9,14 +9,14 @@
     %b.caret
   %ul.dropdown-menu
     %li
-      = link_to search_path(group_id: nil, search: params[:search]) do
+      = link_to search_filter_path(group_id: nil) do
         Any
     - current_user.authorized_groups.sort_by(&:name).each do |group|
       %li
-        = link_to search_path(group_id: group.id, search: params[:search]) do
+        = link_to search_filter_path(group_id: group.id, project_id: nil) do
           = group.name
 
-.dropdown.inline.prepend-left-10
+.dropdown.inline.prepend-left-10.project-filter
   %a.dropdown-toggle.btn.btn-small{href: '#', "data-toggle" => "dropdown"}
     %i.icon-tags
     %span.light Project:
@@ -27,9 +27,9 @@
     %b.caret
   %ul.dropdown-menu
     %li
-      = link_to search_path(project_id: nil, search: params[:search]) do
+      = link_to search_filter_path(project_id: nil) do
         Any
     - current_user.authorized_projects.sort_by(&:name_with_namespace).each do |project|
       %li
-        = link_to search_path(project_id: project.id, search: params[:search]) do
+        = link_to search_filter_path(project_id: project.id, group_id: nil) do
           = project.name_with_namespace

app/views/search/_global_results.html.haml

-.search_results
-  %ul.bordered-list
-    = render partial: "search/results/project", collection: @search_results[:projects]
-    = render partial: "search/results/merge_request", collection: @search_results[:merge_requests]
-    = render partial: "search/results/issue", collection: @search_results[:issues]
+.row
+  .col-sm-3
+    %ul.nav.nav-pills.nav-stacked.search-filter
+      %li{class: ("active" if @scope == 'projects')}
+        = link_to search_filter_path(scope: 'projects') do
+          Projects
+          .pull-right
+            = @search_results.projects_count
+      %li{class: ("active" if @scope == 'issues')}
+        = link_to search_filter_path(scope: 'issues') do
+          Issues
+          .pull-right
+            = @search_results.issues_count
+      %li{class: ("active" if @scope == 'merge_requests')}
+        = link_to search_filter_path(scope: 'merge_requests') do
+          Merge requests
+          .pull-right
+            = @search_results.merge_requests_count
+
+  .col-sm-9
+    .search_results
+      - if @search_results.empty?
+        = render partial: "search/results/empty", locals: { message: "We couldn't find any matching results" }
+
+      %ul.bordered-list.top-list
+        = render partial: "search/results/#{@scope.singularize}", collection: @objects
+      = paginate @objects, theme: 'gitlab'

app/views/search/_project_results.html.haml

-%ul.nav.nav-tabs
-  %li{class: ("active" if params[:search_code].present?)}
-    = link_to search_path(params.merge(search_code: true)) do
-      Repository Code
-  %li{class: ("active" if params[:search_code].blank?)}
-    = link_to search_path(params.merge(search_code: nil)) do
-      Issues and Merge requests
+.row
+  .col-sm-3
+    %ul.nav.nav-pills.nav-stacked.search-filter
+      %li{class: ("active" if @scope == 'blobs')}
+        = link_to search_filter_path(scope: 'blobs') do
+          %i.icon-code
+          Code
+          .pull-right
+            = @search_results.blobs_count
+      %li{class: ("active" if @scope == 'issues')}
+        = link_to search_filter_path(scope: 'issues') do
+          %i.icon-exclamation-sign
+          Issues
+          .pull-right
+            = @search_results.issues_count
+      %li{class: ("active" if @scope == 'merge_requests')}
+        = link_to search_filter_path(scope: 'merge_requests') do
+          %i.icon-code-fork
+          Merge requests
+          .pull-right
+            = @search_results.merge_requests_count
+      %li{class: ("active" if @scope == 'notes')}
+        = link_to search_filter_path(scope: 'notes') do
+          %i.icon-comments
+          Comments
+          .pull-right
+            = @search_results.notes_count
 
-.search_results
-  - if params[:search_code].present?
-    .blob-results
-      - if !@search_results[:blobs].empty?
-        = render partial: "search/results/blob", collection: @search_results[:blobs]
-        = paginate @search_results[:blobs], theme: 'gitlab'
-      - else
-        = render partial: "search/results/empty", :locals => { message: "We couldn't find any matching code" }
-  - else
-    - if @search_results[:merge_requests].present? || @search_results[:issues].present? || @search_results[:notes].present?
-      %ul.bordered-list
-        = render partial: "search/results/merge_request", collection: @search_results[:merge_requests]
-        = render partial: "search/results/issue", collection: @search_results[:issues]
-        = render partial: "search/results/note", collection: @search_results[:notes]
-    - else
-      = render partial: "search/results/empty", locals: { message: "We couldn't find any issues, merge requests or notes" }
+  .col-sm-9
+    .search_results
+      - if @search_results.empty?
+        = render partial: "search/results/empty", locals: { message: "We couldn't find any matching results" }
+
+      %ul.bordered-list.top-list
+        = render partial: "search/results/#{@scope.singularize}", collection: @objects
+      = paginate @objects, theme: 'gitlab'

app/views/search/_results.html.haml

 %h4
-  #{@search_results[:total_results]} results found
+  #{@search_results.total_count} results found
   - if @project
     for #{link_to @project.name_with_namespace, @project}
   - elsif @group
@@ -14,4 +14,3 @@
 
 :javascript
   $(".search_results .term").highlight("#{escape_javascript(params[:search])}");
-

app/views/search/results/_issue.html.haml

 %li
-  issue:
+  %h4
     = link_to [issue.project, issue] do
-    %span ##{issue.iid}
-    %strong.term
-      = truncate issue.title, length: 50
-  %span.light (#{issue.project.name_with_namespace})
+      %span.term.str-truncated= issue.title
+    .pull-right ##{issue.iid}
+  - if issue.description.present?
+    .description.term
+      = preserve do
+        = search_md_sanitize(markdown(issue.description))
+  %span.light
+    #{issue.project.name_with_namespace}
   - if issue.closed?
+    .pull-right
       %span.label.label-danger Closed

app/views/search/results/_merge_request.html.haml

 %li
-  merge request:
+  %h4
     = link_to [merge_request.target_project, merge_request] do
-    %span ##{merge_request.iid}
-    %strong.term
-      = truncate merge_request.title, length: 50
-  - if merge_request.for_fork?
-    %span.light (#{merge_request.source_project.name_with_namespace}:#{merge_request.source_branch} → #{merge_request.target_project.name_with_namespace}:#{merge_request.target_branch})
-  - else
-    %span.light (#{merge_request.source_branch} → #{merge_request.target_branch})
+      %span.term.str-truncated= merge_request.title
+    .pull-right ##{merge_request.iid}
+  - if merge_request.description.present?
+    .description.term
+      = preserve do
+        = search_md_sanitize(markdown(merge_request.description))
+  %span.light
+    #{merge_request.project.name_with_namespace}
+  .pull-right
     - if merge_request.merged?
       %span.label.label-primary Merged
     - elsif merge_request.closed?

app/views/search/results/_note.html.haml

+- project = note.project
 %li
-  note on issue:
-  = link_to [note.project, note.noteable] do
-    %span ##{note.noteable.iid}
-    %strong.term
-      = truncate note.noteable.title, length: 50
-  %span.light (#{note.project.name_with_namespace})
-  - if note.noteable.closed?
-    %span.label Closed
+  %h5.note-search-caption
+    %i.icon-comment
+    = link_to_member(project, note.author, avatar: false)
+    commented on
+
+    - if note.for_commit?
+      = link_to project do
+        = project.name_with_namespace
+      ·
+      = link_to project_commit_path(project, note.commit_id, anchor: dom_id(note)) do
+        Commit #{note.commit_id[0..8]}
+    - else
+      = link_to project do
+        = project.name_with_namespace
+      ·
+      %span #{note.noteable_type.titleize} ##{note.noteable.iid}
+      ·
+      = link_to [project, note.noteable, anchor: dom_id(note)]  do
+        = note.noteable.title
+
+  .note-search-result
+    .term
+      = preserve do
+        = search_md_sanitize(markdown(note.note, {no_header_anchors: true}))

app/views/search/results/_project.html.haml

 %li
-  project:
+  %h4
     = link_to project do
-    %strong.term= project.name_with_namespace
+      %span.term= project.name_with_namespace
   - if project.description.present?
-    –
     %span.light.term= project.description

app/views/search/show.html.haml

@@ -13,7 +13,7 @@
         = render 'filter', f: f
       = hidden_field_tag :project_id, params[:project_id]
       = hidden_field_tag :group_id, params[:group_id]
-      = hidden_field_tag :search_code, params[:search_code]
+      = hidden_field_tag :scope, params[:scope]
 
   .results.prepend-top-10
     - if params[:search].present?

features/dashboard/search.feature

-@dashboard
-Feature: Dashboard Search
-  Background:
-    Given I sign in as a user
-    And I own project "Shop"
-    And I visit dashboard search page
-
-  Scenario: I should see project I am looking for
-    Given I search for "Sho"
-    Then I should see "Shop" project link

features/search.feature

+@dashboard
+Feature: Search
+  Background:
+    Given I sign in as a user
+    And I own project "Shop"
+    And I visit dashboard search page
+
+  Scenario: I should see project I am looking for
+    Given I search for "Sho"
+    Then I should see "Shop" project link
+
+  Scenario: I should see issues I am looking for
+    And project has issues
+    When I search for "Foo"
+    And I click "Issues" link
+    Then I should see "Foo" link
+    And I should not see "Bar" link
+
+  Scenario: I should see merge requests I am looking for
+    And project has merge requests
+    When I search for "Foo"
+    When I click "Merge requests" link
+    Then I should see "Foo" link
+    And I should not see "Bar" link
+
+  Scenario: I should see project code I am looking for
+    When I click project "Shop" link
+    And I search for "rspec"
+    Then I should see code results for project "Shop"
+
+  Scenario: I should see project issues
+    And project has issues
+    When I click project "Shop" link
+    And I search for "Foo"
+    And I click "Issues" link
+    Then I should see "Foo" link
+    And I should not see "Bar" link
+
+  Scenario: I should see project merge requests
+    And project has merge requests
+    When I click project "Shop" link
+    And I search for "Foo"
+    And I click "Merge requests" link
+    Then I should see "Foo" link
+    And I should not see "Bar" link
+

features/steps/dashboard/search.rb

-class DashboardSearch < Spinach::FeatureSteps
-  include SharedAuthentication
-  include SharedPaths
-  include SharedProject
-
-  Given 'I search for "Sho"' do
-    fill_in "dashboard_search", with: "Sho"
-    click_button "Search"
-  end
-
-  Then 'I should see "Shop" project link' do
-    page.should have_link "Shop"
-  end
-
-  Given 'I search for "Contibuting"' do
-    fill_in "dashboard_search", with: "Contibuting"
-    click_button "Search"
-  end
-end

features/steps/project/search_code.rb

@@ -6,7 +6,6 @@ class ProjectSearchCode < Spinach::FeatureSteps
   step 'I search for term "coffee"' do
     fill_in "search", with: "coffee"
     click_button "Go"
-    click_link 'Repository Code'
   end
 
   step 'I should see files from repository containing "coffee"' do
@@ -15,6 +14,6 @@ class ProjectSearchCode < Spinach::FeatureSteps
   end
 
   step 'I should see empty result' do
-    page.should have_content "We couldn't find any matching code"
+    page.should have_content "We couldn't find any matching"
   end
 end

features/steps/search.rb

+class Spinach::Features::Search < Spinach::FeatureSteps
+  include SharedAuthentication
+  include SharedPaths
+  include SharedProject
+
+  step 'I search for "Sho"' do
+    fill_in "dashboard_search", with: "Sho"
+    click_button "Search"
+  end
+
+  step 'I search for "Foo"' do
+    fill_in "dashboard_search", with: "Foo"
+    click_button "Search"
+  end
+
+  step 'I search for "rspec"' do
+    fill_in "dashboard_search", with: "rspec"
+    click_button "Search"
+  end
+
+  step 'I click "Issues" link' do
+    within '.search-filter' do
+      click_link 'Issues'
+    end
+  end
+
+  step 'I click project "Shop" link' do
+    within '.project-filter' do
+      click_link project.name_with_namespace
+    end
+  end
+
+  step 'I click "Merge requests" link' do
+    within '.search-filter' do
+      click_link 'Merge requests'
+    end
+  end
+
+  step 'I should see "Shop" project link' do
+    page.should have_link "Shop"
+  end
+
+  step 'I should see code results for project "Shop"' do
+    page.should have_content 'Update capybara, rspec-rails, poltergeist to recent versions'
+  end
+
+  step 'I search for "Contibuting"' do
+    fill_in "dashboard_search", with: "Contibuting"
+    click_button "Search"
+  end
+
+  step 'project has issues' do
+    create(:issue, title: "Foo", project: project)
+    create(:issue, title: "Bar", project: project)
+  end
+
+  step 'project has merge requests' do
+    create(:merge_request, title: "Foo", source_project: project, target_project: project)
+    create(:merge_request, :simple, title: "Bar", source_project: project, target_project: project)
+  end
+
+  step 'I should see "Foo" link' do
+    page.should have_link "Foo"
+  end
+
+  step 'I should not see "Bar" link' do
+    page.should_not have_link "Bar"
+  end
+
+  def project
+    @project ||= Project.find_by(name: "Shop")
+  end
+end

lib/gitlab/project_search_results.rb

+module Gitlab
+  class ProjectSearchResults < SearchResults
+    attr_reader :project, :repository_ref
+
+    def initialize(project_id, query, repository_ref = nil)
+      @project = Project.find(project_id)
+      @repository_ref = repository_ref
+      @query = Shellwords.shellescape(query) if query.present?
+    end
+
+    def objects(scope, page = nil)
+      case scope
+      when 'notes'
+        notes.page(page).per(per_page)
+      when 'blobs'
+        Kaminari.paginate_array(blobs).page(page).per(per_page)
+      else
+        super
+      end
+    end
+
+    def total_count
+      @total_count ||= issues_count + merge_requests_count + blobs_count + notes_count
+    end
+
+    def blobs_count
+      @blobs_count ||= blobs.count
+    end
+
+    def notes_count
+      @notes_count ||= notes.count
+    end
+
+    private
+
+    def blobs
+      if project.empty_repo?
+        []
+      else
+        project.repository.search_files(query, repository_ref)
+      end
+    end
+
+    def notes
+      Note.where(project_id: limit_project_ids).search(query).order('updated_at DESC')
+    end
+
+    def limit_project_ids
+      [project.id]
+    end
+  end
+end

lib/gitlab/search_results.rb

+module Gitlab
+  class SearchResults
+    attr_reader :query
+
+    # Limit search results by passed project ids
+    # It allows us to search only for projects user has access to
+    attr_reader :limit_project_ids
+
+    def initialize(limit_project_ids, query)
+      @limit_project_ids = limit_project_ids || Project.all
+      @query = Shellwords.shellescape(query) if query.present?
+    end
+
+    def objects(scope, page = nil)
+      case scope
+      when 'projects'
+        projects.page(page).per(per_page)
+      when 'issues'
+        issues.page(page).per(per_page)
+      when 'merge_requests'
+        merge_requests.page(page).per(per_page)
+      else
+        Kaminari.paginate_array([]).page(page).per(per_page)
+      end
+    end
+
+    def total_count
+      @total_count ||= projects_count + issues_count + merge_requests_count
+    end
+
+    def projects_count
+      @projects_count ||= projects.count
+    end
+
+    def issues_count
+      @issues_count ||= issues.count
+    end
+
+    def merge_requests_count
+      @merge_requests_count ||= merge_requests.count
+    end
+
+    def empty?
+      total_count.zero?
+    end
+
+    private
+
+    def projects
+      Project.where(id: limit_project_ids).search(query)
+    end
+
+    def issues
+      Issue.where(project_id: limit_project_ids).full_search(query).order('updated_at DESC')
+    end
+
+    def merge_requests
+      MergeRequest.in_projects(limit_project_ids).full_search(query).order('updated_at DESC')
+    end
+
+    def default_scope
+      'projects'
+    end
+
+    def per_page
+      20
+    end
+  end
+end

spec/services/search_service_spec.rb

@@ -19,7 +19,7 @@ describe 'Search::GlobalService' do
       it 'should return public projects only' do
         context = Search::GlobalService.new(nil, search: "searchable")
         results = context.execute
-        results[:projects].should match_array [public_project]
+        results.objects('projects').should match_array [public_project]
       end
     end
 
@@ -27,19 +27,19 @@ describe 'Search::GlobalService' do
       it 'should return public, internal and private projects' do
         context = Search::GlobalService.new(user, search: "searchable")
         results = context.execute
-        results[:projects].should match_array [public_project, found_project, internal_project]
+        results.objects('projects').should match_array [public_project, found_project, internal_project]
       end
 
       it 'should return only public & internal projects' do
         context = Search::GlobalService.new(internal_user, search: "searchable")
         results = context.execute
-        results[:projects].should match_array [internal_project, public_project]
+        results.objects('projects').should match_array [internal_project, public_project]
       end
 
       it 'namespace name should be searchable' do
         context = Search::GlobalService.new(user, search: found_project.namespace.path)
         results = context.execute
-        results[:projects].should match_array [found_project]
+        results.objects('projects').should match_array [found_project]
       end
     end
   end