Merge branch '275899-share-view-code-for-mr-issue-search-results' into 'master'

Extract shared view search/results/_issuable.html.haml

See merge request gitlab-org/gitlab!46944

app/controllers/search_controller.rb

@@ -9,6 +9,7 @@ class SearchController < ApplicationController
   SCOPE_PRELOAD_METHOD = {
     projects: :with_web_entity_associations,
     issues: :with_web_entity_associations,
+    merge_requests: :with_web_entity_associations,
     epics: :with_web_entity_associations
   }.freeze
 

app/helpers/search_helper.rb

@@ -367,10 +367,10 @@ module SearchHelper
   end
 
   # _search_highlight is used in EE override
-  def highlight_and_truncate_issue(issue, search_term, _search_highlight)
-    return unless issue.description.present?
+  def highlight_and_truncate_issuable(issuable, search_term, _search_highlight)
+    return unless issuable.description.present?
 
-    simple_search_highlight_and_truncate(issue.description, search_term, highlighter: '<span class="gl-text-black-normal gl-font-weight-bold">\1</span>')
+    simple_search_highlight_and_truncate(issuable.description, search_term, highlighter: '<span class="gl-text-black-normal gl-font-weight-bold">\1</span>')
   end
 
   def show_user_search_tab?
@@ -380,6 +380,36 @@ module SearchHelper
       can?(current_user, :read_users_list)
     end
   end
+
+  def issuable_state_to_badge_class(issuable)
+    # Closed is considered "danger" for MR so we need to handle separately
+    if issuable.is_a?(::MergeRequest)
+      if issuable.merged?
+        :primary
+      elsif issuable.closed?
+        :danger
+      else
+        :success
+      end
+    else
+      if issuable.closed?
+        :info
+      else
+        :success
+      end
+    end
+  end
+
+  def issuable_state_text(issuable)
+    case issuable.state
+    when 'merged'
+      _("Merged")
+    when 'closed'
+      _("Closed")
+    else
+      _("Open")
+    end
+  end
 end
 
 SearchHelper.prepend_if_ee('EE::SearchHelper')

app/models/merge_request.rb

@@ -294,6 +294,7 @@ class MergeRequest < ApplicationRecord
   scope :preload_author, -> { preload(:author) }
   scope :preload_approved_by_users, -> { preload(:approved_by_users) }
   scope :preload_metrics, -> (relation) { preload(metrics: relation) }
+  scope :with_web_entity_associations, -> { preload(:author, :target_project) }
 
   scope :with_auto_merge_enabled, -> do
     with_state(:opened).where(auto_merge_enabled: true)

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

+%div{ class: 'search-result-row gl-pb-3! gl-mt-5 gl-mb-0!' }
+  %span.gl-display-flex.gl-align-items-center
+    %span.badge.badge-pill.gl-badge.sm{ class: "badge-#{issuable_state_to_badge_class(issuable)}" }= issuable_state_text(issuable)
+    = sprite_icon('eye-slash', css_class: 'gl-text-gray-500 gl-ml-2') if issuable.respond_to?(:confidential?) && issuable.confidential?
+    = link_to issuable_path(issuable), data: { track_event: 'click_text', track_label: "#{issuable.class.name.downcase}_title", track_property: 'search_result' }, class: 'gl-w-full' do
+      %span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issuable.title
+  .gl-text-gray-500.gl-my-3
+    = sprintf(s_(' %{project_name}#%{issuable_iid} · opened %{issuable_created} by %{author}'), { project_name: issuable.project.full_name, issuable_iid: issuable.iid, issuable_created: time_ago_with_tooltip(issuable.created_at, placement: 'bottom'), author: link_to_member(@project, issuable.author, avatar: false) }).html_safe
+  .description.term.col-sm-10.gl-px-0
+    = highlight_and_truncate_issuable(issuable, @search_term, @search_highlight)

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

-%div{ class: 'search-result-row gl-pb-3! gl-mt-5 gl-mb-0!' }
-  %span.gl-display-flex.gl-align-items-center
-    - if issue.closed?
-      %span.badge.badge-info.badge-pill.gl-badge.sm= _("Closed")
-    - else
-      %span.badge.badge-success.badge-pill.gl-badge.sm= _("Open")
-    = sprite_icon('eye-slash', css_class: 'gl-text-gray-500 gl-ml-2') if issue.confidential?
-    = link_to project_issue_path(issue.project, issue), data: { track_event: 'click_text', track_label: 'issue_title', track_property: 'search_result' }, class: 'gl-w-full' do
-      %span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issue.title
-  .gl-text-gray-500.gl-my-3
-    = sprintf(s_(' %{project_name}#%{issue_iid} · opened %{issue_created} by %{author}'), { project_name: issue.project.full_name, issue_iid: issue.iid, issue_created: time_ago_with_tooltip(issue.created_at, placement: 'bottom'), author: link_to_member(@project, issue.author, avatar: false) }).html_safe
-  .description.term.col-sm-10.gl-px-0
-    = highlight_and_truncate_issue(issue, @search_term, @search_highlight)
+= render partial: 'search/results/issuable', object: issue

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

-.search-result-row
-  %h4
-    = link_to project_merge_request_path(merge_request.target_project, merge_request), data: {track_event: 'click_text', track_label: 'merge_request_title', track_property: 'search_result'} do
-      %span.term.str-truncated= merge_request.title
-    - if merge_request.merged?
-      %span.badge.badge-primary.gl-ml-2= _("Merged")
-    - elsif merge_request.closed?
-      %span.badge.badge-danger.gl-ml-2= _("Closed")
-    .float-right= merge_request.to_reference
-  - if merge_request.description.present?
-    .description.term
-      = search_md_sanitize(merge_request.description)
-  %span.light
-    #{merge_request.project.full_name}
+= render partial: 'search/results/issuable', object: merge_request

changelogs/unreleased/275899-share-view-code-for-mr-issue-search-results.yml

+---
+title: Update merge request search results design
+merge_request: 46944
+author:
+type: changed

ee/app/helpers/ee/search_helper.rb

@@ -69,14 +69,14 @@ module EE
       end
     end
 
-    override :highlight_and_truncate_issue
-    def highlight_and_truncate_issue(issue, search_term, search_highlight)
-      return super unless search_service.use_elasticsearch? && search_highlight[issue.id]&.description.present?
+    override :highlight_and_truncate_issuable
+    def highlight_and_truncate_issuable(issuable, search_term, search_highlight)
+      return super unless search_service.use_elasticsearch? && search_highlight[issuable.id]&.description.present?
 
       # We use Elasticsearch highlighting for results from Elasticsearch. Sanitize the description, replace the
       # pre/post tags from Elasticsearch with highlighting, truncate, and mark as html_safe. HTML tags are not
       # counted towards the character limit.
-      text = sanitize(search_highlight[issue.id].description.first)
+      text = sanitize(search_highlight[issuable.id].description.first)
       text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_START_TAG, '<span class="gl-text-black-normal gl-font-weight-bold">')
       text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_END_TAG, '</span>')
       Truncato.truncate(text, count_tags: false, count_tail: false, max_length: 200).html_safe

ee/spec/helpers/search_helper_spec.rb

@@ -209,7 +209,7 @@ RSpec.describe SearchHelper do
     end
   end
 
-  describe '#highlight_and_truncate_issue' do
+  describe '#highlight_and_truncate_issuable' do
     let(:description) { 'hello world' }
     let(:issue) { create(:issue, description: description) }
     let(:user) { create(:user) }
@@ -221,7 +221,7 @@ RSpec.describe SearchHelper do
     end
 
     # Elasticsearch returns Elasticsearch::Model::HashWrapper class for the highlighting
-    subject { highlight_and_truncate_issue(issue, 'test', Elasticsearch::Model::HashWrapper.new(search_highlight)) }
+    subject { highlight_and_truncate_issuable(issue, 'test', Elasticsearch::Model::HashWrapper.new(search_highlight)) }
 
     context 'when description is not present' do
       let(:description) { nil }

locale/gitlab.pot

@@ -16,7 +16,7 @@ msgstr ""
 "Content-Transfer-Encoding: 8bit\n"
 "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n"
 
-msgid " %{project_name}#%{issue_iid} · opened %{issue_created} by %{author}"
+msgid " %{project_name}#%{issuable_iid} · opened %{issuable_created} by %{author}"
 msgstr ""
 
 msgid " %{start} to %{end}"

spec/helpers/search_helper_spec.rb

@@ -477,7 +477,7 @@ RSpec.describe SearchHelper do
     end
   end
 
-  describe '#highlight_and_truncate_issue' do
+  describe '#highlight_and_truncate_issuable' do
     let(:description) { 'hello world' }
     let(:issue) { create(:issue, description: description) }
     let(:user) { create(:user) }
@@ -486,7 +486,7 @@ RSpec.describe SearchHelper do
       allow(self).to receive(:current_user).and_return(user)
     end
 
-    subject { highlight_and_truncate_issue(issue, 'test', {}) }
+    subject { highlight_and_truncate_issuable(issue, 'test', {}) }
 
     context 'when description is not present' do
       let(:description) { nil }
@@ -545,4 +545,38 @@ RSpec.describe SearchHelper do
       end
     end
   end
+
+  describe '#issuable_state_to_badge_class' do
+    context 'with merge request' do
+      it 'returns correct badge based on status' do
+        expect(issuable_state_to_badge_class(build(:merge_request, :merged))).to eq(:primary)
+        expect(issuable_state_to_badge_class(build(:merge_request, :closed))).to eq(:danger)
+        expect(issuable_state_to_badge_class(build(:merge_request, :opened))).to eq(:success)
+      end
+    end
+
+    context 'with an issue' do
+      it 'returns correct badge based on status' do
+        expect(issuable_state_to_badge_class(build(:issue, :closed))).to eq(:info)
+        expect(issuable_state_to_badge_class(build(:issue, :opened))).to eq(:success)
+      end
+    end
+  end
+
+  describe '#issuable_state_text' do
+    context 'with merge request' do
+      it 'returns correct badge based on status' do
+        expect(issuable_state_text(build(:merge_request, :merged))).to eq(_('Merged'))
+        expect(issuable_state_text(build(:merge_request, :closed))).to eq(_('Closed'))
+        expect(issuable_state_text(build(:merge_request, :opened))).to eq(_('Open'))
+      end
+    end
+
+    context 'with an issue' do
+      it 'returns correct badge based on status' do
+        expect(issuable_state_text(build(:issue, :closed))).to eq(_('Closed'))
+        expect(issuable_state_text(build(:issue, :opened))).to eq(_('Open'))
+      end
+    end
+  end
 end

spec/requests/search_controller_spec.rb

@@ -45,14 +45,9 @@ RSpec.describe SearchController, type: :request do
       let(:object) { :merge_request }
       let(:creation_args) { { source_project: project, title: 'bar' } }
       let(:params) { { search: 'bar', scope: 'merge_requests' } }
-      # some N+1 queries exist
-      # each merge request require 4 extra queries for:
-      # - one for projects
-      # - one for namespaces
-      # - two for routes
-      # plus 4 additional queries run for the logged in user:
+      # there are 4 additional queries run for the logged in user:
       #   - (1) geo_nodes, (1) users, (2) broadcast_messages
-      let(:threshold) { 16 }
+      let(:threshold) { 4 }
 
       it_behaves_like 'an efficient database result'
     end